fix one-tick-race condition in asyncMap #11249

phryneas · 2023-09-25T14:40:29Z

Fixes #11149
(hopefully 🤞 )

Can be tried out in https://github.com/quocluongha/rn-apollo-test - I can't reproduce it in the browser. It seems this specific timing only triggers in React Native.

I'll try to explain this one in comments...

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests
(This seems impossible to test for, but trying out in the linked repo it seems to fix the bug)

changeset-bot · 2023-09-25T14:40:32Z

🦋 Changeset detected

Latest commit: 591d6c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-09-25T14:41:53Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.1 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.45 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	41.99 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.53 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.27 KB (+0.02% 🔺)
import { ApolloProvider } from "dist/react/index.js"	1.21 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.2 KB (0%)
import { useQuery } from "dist/react/index.js"	4.27 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.58 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.4 KB (0%)
import { useMutation } from "dist/react/index.js"	2.52 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.51 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.59 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.02 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.1 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.52 KB (0%)
import { useReadQuery } from "dist/react/index.js"	2.96 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	2.91 KB (0%)
import { useFragment } from "dist/react/index.js"	2.08 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.03 KB (0%)

phryneas · 2023-09-25T14:45:37Z

src/utilities/observables/asyncMap.ts

              },
              (error) => {
-                --activeCallbackCount;
                throw error;
              }
            )
            .catch((caught) => {
              error && error.call(observer, caught);
            });


The race condition essentially is the following:

handler.next (line 58) is called by the upstream Observable

code starts flowing in .then(both, both), both (the mapping function) throws an error (as it should in this specific scenario, because the result from the server contains errors and both is the function to handle that)

code reaches the --activeCallbackCount in line 43 and waits a tick before executing error.call(observer, caught)

meanwhile, handler.complete is called by the upstream Observable

activeCallbackCount is currently 0, complete.call(observer) executes

⚠️ now it doesn't matter anymore if error.call(observer, caught) executes or not - the downstream observable is already complete (in our case without data, since we went into the error case, not the success case, so next was never called).

phryneas · 2023-09-25T14:50:04Z

src/utilities/observables/asyncMap.ts

+          promiseQueue.finally(() => {
+            --activeCallbackCount;
+            if (completed) {
+              handler.complete!();
+            }
+          });


To fix this, --activeCallbackCount has to be executed after next.call(observer, result)/error.call(observer, caught) above had a chance to execute.

I moved this out of the promise chain that is assigned to promiseQueue, as we don't need to queue up another tick in case promiseQueue is reused within a very short time span.

I agree with this logic, but I'm sure our future selves would appreciate a comment about why the .finally isn't just chained after the .catch.

phryneas · 2023-09-25T14:50:23Z

/release:pr

github-actions · 2023-09-25T14:52:03Z

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

benjamn

I support this general solution, and I appreciate your careful reasoning, but have one question (and a couple suggestions) for you to consider.

benjamn · 2023-09-25T15:50:49Z

src/utilities/observables/asyncMap.ts

+          promiseQueue.finally(() => {
+            --activeCallbackCount;
+            if (completed) {
+              handler.complete!();
+            }
+          });


I agree with this logic, but I'm sure our future selves would appreciate a comment about why the .finally isn't just chained after the .catch.

benjamn · 2023-09-25T15:53:58Z

src/utilities/observables/asyncMap.ts

+          promiseQueue.finally(() => {
+            --activeCallbackCount;
+            if (completed) {
+              handler.complete!();


Suggested change

promiseQueue.finally(() => {

--activeCallbackCount;

if (completed) {

handler.complete!();

promiseQueue.finally(() => {

--activeCallbackCount;

// Because of the .finally delay, it's possible handler.complete() gets

// called shortly before activeCallbackCount falls to 0, so each time we

// decrement activeCallbackCount we give handler.complete() another

// chance to call observer.complete().

if (completed) {

handler.complete!();

This might be a helpful comment, given the racy nature of this code.

benjamn · 2023-09-25T15:57:30Z

src/utilities/observables/asyncMap.ts

            .then(
              (result) => {
-                --activeCallbackCount;
                next && next.call(observer, result);
-                if (completed) {
-                  handler.complete!();
-                }
              },
              (error) => {
-                --activeCallbackCount;
                throw error;
              }


We were not previously calling handler.complete() in the error handler, but now the promiseQueue.finally callback is potentially calling handler.complete() whenever we decrement activeCallbackCount. Is that a problem? Specifically, is it possible we might call error.call(observer, caught) and then also call complete.call(observer)?

phryneas · 2023-09-28T16:40:58Z

I talked about this with @benjamn , and there is nothing blocking us from doing #11252 instead of this PR, so I'll close this one.

phryneas added 2 commits September 25, 2023 16:37

fix one-tick-race condition in asyncMap

7e0695a

changeset

984e474

phryneas commented Sep 25, 2023

View reviewed changes

phryneas mentioned this pull request Sep 25, 2023

useLazyQuery throw error when error data is null or undefined #11149

Closed

size-limit

591d6c1

phryneas requested review from benjamn, jerelmiller and alessbell September 25, 2023 15:17

phryneas marked this pull request as ready for review September 25, 2023 15:18

phryneas self-assigned this Sep 25, 2023

benjamn reviewed Sep 25, 2023

View reviewed changes

This was referenced Sep 26, 2023

Cannot read property 'data' of undefined when using useLazyQuery #11251

Closed

simplify asyncMap implementation #11252

Merged

phryneas closed this Sep 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix one-tick-race condition in asyncMap #11249

fix one-tick-race condition in asyncMap #11249

phryneas commented Sep 25, 2023 •

edited

Loading

changeset-bot bot commented Sep 25, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023 •

edited

Loading

phryneas Sep 25, 2023 •

edited

Loading

phryneas Sep 25, 2023

benjamn Sep 25, 2023

phryneas commented Sep 25, 2023

github-actions bot commented Sep 25, 2023

benjamn left a comment

benjamn Sep 25, 2023

benjamn Sep 25, 2023

benjamn Sep 25, 2023

phryneas commented Sep 28, 2023

fix one-tick-race condition in asyncMap #11249

fix one-tick-race condition in asyncMap #11249

Conversation

phryneas commented Sep 25, 2023 • edited Loading

Checklist:

changeset-bot bot commented Sep 25, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Sep 25, 2023 • edited Loading

size-limit report 📦

phryneas Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

phryneas Sep 25, 2023

Choose a reason for hiding this comment

benjamn Sep 25, 2023

Choose a reason for hiding this comment

phryneas commented Sep 25, 2023

github-actions bot commented Sep 25, 2023

benjamn left a comment

Choose a reason for hiding this comment

benjamn Sep 25, 2023

Choose a reason for hiding this comment

benjamn Sep 25, 2023

Choose a reason for hiding this comment

benjamn Sep 25, 2023

Choose a reason for hiding this comment

phryneas commented Sep 28, 2023

phryneas commented Sep 25, 2023 •

edited

Loading

changeset-bot bot commented Sep 25, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023 •

edited

Loading

phryneas Sep 25, 2023 •

edited

Loading